Sorry the formats in this file were done in a CI-specific PR we merged, go ahead and rebase and drop them thank you :)

@Harshdev098 sorry i take it back, don't rebase to a new commit just yet :) if you can just drop these changes, keep the same base commit on main, will rebase to a newer base commit as necessary thank you

Ohh sorry @tankyleo , but I have done it already, my branch is now up to date with main

@Harshdev098 want to make sure you know about git reflog ? Would have allowed you to revert it back easily before pushing but all good now !

We also do not need to build VssDbRecords for the delete_items, so let's delete those.

Ohh sorry @tankyleo , but I have done it already, my branch is now up to date with main

Can you share why you picked an RwLock here ? I always default to Mutex unless it will be read-heavy / have good reasons to use RwLock.

VSS is rather even-handed / write-heavy, so my gut instinct is to go for Mutex.

Actually have used Mutex earlier but have changed to RwLock for debugging the integration test that was failing!

sounds good ok would you consider switching back to Mutex ? or are there some bugs that come up specifically when using Mutex ?

Switching back to Mutex, it was deadlock issue at that time!

Also I found that testing the ldk node integration test against memory_store I need to restart the server to test after every updation, because I think starting the integration test it needs fresh data but it gives error when runned multiple times without restarting the server, which can make bad experience as a developer @tankyleo!?

@Harshdev098 does this have to do with the choice between Mutex and RwLock ? I'll look into this

Probably not, you can reproduce it by just running the ldk node integration test multiple times with the same server without restarting it. I think it could be a problem with test because every time we run the test and its failing while setting up the first node and it's giving on Error value
ReadFailed

have you had success with passing the ldk integration test against in_memory just once ? I've passed against postgres all good, but in_memory_store seems to hang.

Ohh sorry, it was my fault but I have updated the pr with the required changes, and yah I have also suffered of this hang but underestimated it because its not happening regularly like with the same code sometimes, it's passes the integration test and sometimes just stuck to it, the same was happening with ldk test against postgres too in CI @tankyleo @tnull

Ohh sorry, it was my fault but I have updated the pr with the required changes, and yah I have also suffered of this hang but underestimated it because its not happening regularly like with the same code sometimes, it's passes the integration test and sometimes just stuck to it, the same was happening with ldk test against postgres too in CI @tankyleo @tnull

Yes, this might happen on current LDK Node main. It should be fixed as soon as lightningdevkit/ldk-node#627 lands, which should happen shortly/in the few days.

How about we use a BTreeMap here to avoid having to resort all the keys on every subsequent call to list_key_versions further down below ?

Was this specific flag requested by a user ? I'd be in favor of deleting it, and keeping the in_memory configuration to a single spot, as we currently have it in the configuration file.

But let me know what you think, curious to hear your thoughts.

I added --in-memory to make it easy for external projects like Fedimint to run integration tests with our VSS server

sounds good ok and the configuration file would be too much of a hassle for them ?

Yah, exactly

or we could change the "default" database to "in_memory" store so people can clone the repository, and run CI directly right ?

Also another question: could Fedimint just run against postgres ? We've done some work in the past to make setting that up much easier, as close to "just run it". Thank you for the context. You can see our own build-and-deploy-rust.yml in this repo runs test against a postgres service without too much trouble.

yah they can run it in postgres but the in-memory simplifies their devimint CI like no DB service needed, just spawn the binary,
And the in-memory flag, it's low-effort and keeps config as the single source while enabling overrides. I don't think to make the in_memory the default one!

ok sound sgood let's keep things as you've got them now thank you

Should we consider using a StoreType enum instead of String?

#[derive(Deserialize)]
pub(crate) enum StoreType {
    #[serde(rename = "postgres")]
    Postgres,
    #[serde(rename = "in-memory")]
    InMemory,
}

We'd get:

1. Better type safety because invalid values would be rejected when we parse the configuration parameters and not latter downstream.
2. A self-documenting type that clearly defines valid options without requiring inline comments.
3. Exhaustive matching that handles the variants only without needing to handle the wildcard _ arm as is currently done with.

   vss-server/rust/server/src/main.rs

   Lines 101 to 104 in 02a1b4e

   	_ => {
   	eprintln!("Invalid backend_type: {}. Must be 'postgres' or 'in_memory'", config.server_config.store_type);
   	std::process::exit(1);
   	},

4. Refactoring safety when/if we add new store types as this will produce compile errors at all usage sites.

The load_config function remains unchanged since serde handles the deserialization automatically.

Ofcourse, better way to handle that! Thanks.

As already mentioned, I don't think we should have configuration-related failures here.

This would need to be modified if you accept the StoreType enum recommendation.

nit: Would you consider grouping modules by visibility, i.e. public together? This is just a style preference so up to you to decide.

Should VssDbRecord remain in postgres_store? Currently, in_memory_store importing from postgres_store creates a false dependency and suggests the type is PostgreSQL-specific rather than a shared abstraction. Would you consider moving it to a models.rs file (which would also provide a home for other shared types)?

Sure